Skip to content

refactor: replace InternedString with Cow in IndexPackage #15559

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

0xPoe
Copy link
Member

@0xPoe 0xPoe commented May 18, 2025

What does this PR try to resolve?

ref #14834

As described in the issue, we want to move IndexPackage into cargo-util-schemas. However, it contains InternedString fields, which we don't want to expose as part of the public API.
This PR replaces InternedString with Cow.

How should we test and review this PR?

It shouldn't change or break any tests.

Additional information

None

@rustbot rustbot added the A-registries Area: registries label May 18, 2025
@0xPoe 0xPoe changed the title refactor: replace InternedString with Cow in IndexPackage WIP: refactor: replace InternedString with Cow in IndexPackage May 18, 2025
@0xPoe 0xPoe force-pushed the rustin-patch-cow branch from eaf777e to f290a45 Compare May 18, 2025 20:32
@0xPoe 0xPoe force-pushed the rustin-patch-cow branch 2 times, most recently from 5fdaca9 to de4a534 Compare May 19, 2025 20:45
@0xPoe 0xPoe changed the title WIP: refactor: replace InternedString with Cow in IndexPackage refactor: replace InternedString with Cow in IndexPackage May 19, 2025
@0xPoe 0xPoe marked this pull request as ready for review May 19, 2025 20:50
@rustbot
Copy link
Collaborator

rustbot commented May 19, 2025

r? @ehuss

rustbot has assigned @ehuss.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 19, 2025
@0xPoe 0xPoe assigned weihanglo and unassigned ehuss May 19, 2025
@ehuss
Copy link
Contributor

ehuss commented May 19, 2025

I would recommend at least doing some benchmarking using our bench suite. https://github.com/rust-lang/cargo/tree/master/benches contains some documentation, and particularly the resolve bench. cargo bench -p benchsuite --bench resolve is a good place to start (while you are figuring out how to run the benchmarks you can do an individual workspace like resolve_ws/rust to speed things up), and Comparing implementation explains how you can do a comparison report against master.

@0xPoe 0xPoe force-pushed the rustin-patch-cow branch from de4a534 to c5b799e Compare May 22, 2025 14:41
@0xPoe

This comment was marked as outdated.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we could change InternString::from_cow to something like:

impl<'a> From<Cow<'a, str>> for InternedString {
    fn from(cs: Cow<'a, str>) -> Self {
        let mut cache = interned_storage();
        let s = cache.get(cs.as_ref()).copied().unwrap_or_else(|| {
            let s = cs.into_owned().leak();
            cache.insert(s);
            s
        });

        InternedString { inner: s }
    }
}

So in other places we can rely on Into<InternedString>.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@weihanglo Do you think it makes sense to replace all instances of InternedString::new with .into() where possible? There are quite a few usages of InternedString::new.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am fine with wherever reasonable. Could also leave them for future

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am fine with wherever reasonable. Could also leave them for future

@0xPoe 0xPoe force-pushed the rustin-patch-cow branch from c5b799e to 42eb272 Compare May 23, 2025 14:24
@0xPoe 0xPoe marked this pull request as draft May 23, 2025 20:17
@rustbot rustbot added the A-rebuild-detection Area: rebuild detection and fingerprinting label May 26, 2025
@0xPoe 0xPoe force-pushed the rustin-patch-cow branch from 94aac2e to 5fe108d Compare May 26, 2025 14:54
@0xPoe
Copy link
Member Author

0xPoe commented May 27, 2025

I’ve tested this change several times using the resolve_ws/rust benchmark suite.

My test env:

  Model Name:	MacBook Pro
  Chip:	Apple M4 Max
  Total Number of Cores:	14 (10 performance and 4 efficiency)
  Memory:	36 GB

Test result:
master commit: HEAD is now at c2c636a fix(toml): Remove workaround for rustc frontmatter support (#15570)
master: cargo bench -p benchsuite --bench resolve -- resolve_ws/rust --save-baseline master --measurement-time 50 --sample-size 500
patch: cargo bench -p benchsuite --bench resolve -- resolve_ws/rust --baseline master --measurement-time 50 --sample-size 500

First round:

Benchmark Time (mean) Change Outliers
resolve_ws/rust 91.910 ms -0.9313% 31 (6.20%) - 14 high mild, 16 high severe
resolve_ws/rust-ws-inherit 92.021 ms +0.7503% 43 (8.60%) - 14 high mild, 22 high severe

Second round:

Benchmark Time (mean) Change Outliers
resolve_ws/rust 92.142 ms +1.7303% 33 (6.60%) - 10 high mild, 23 high severe
resolve_ws/rust-ws-inherit 91.193 ms -0.0707% 30 (6.00%) - 8 high mild, 22 high severe

Third round:

Benchmark Time (mean) Change Outliers
resolve_ws/rust 91.398 ms +1.2693% 28 (5.60%) - 8 high mild, 20 high severe
resolve_ws/rust-ws-inherit 90.558 ms +0.5349% 26 (5.20%) - 8 high mild, 18 high severe

Fourth round:

Benchmark Time (mean) Change Outliers
resolve_ws/rust 91.193 ms +0.8302% 26 (5.20%) - 11 high mild, 15 high severe
resolve_ws/rust-ws-inherit 90.591 ms +0.8267% 34 (6.80%) - 6 high mild, 17 high severe

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rebuild-detection Area: rebuild detection and fingerprinting A-registries Area: registries S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants